Perf. improvements with activation and cell merge. #16251
Conversation
|
Openned rowPinning sample and pinned a row: |
|
@dkamburov First one, I was not able to reproduce: Are you sure you have the latest changes in the branch or are there any additional steps needed to reproduce? Second one is a sample issue. Because of the way the data is added, the same object refs are used in the grid sample: Hence, duplicate keys. I fixed it in the sample. |
|
@MayaKirova appologize on the non reproducible sample: I do had some changes on that sample(didn't noticed them at first as they were from my previous testing on the cell merging):
|
There was a problem hiding this comment.
Additional, issues I found:
- If you click on any of the headers in the dev sample an error is thrown.
- Go to the performance project and enable the merging on all columns. Run the performance samples using
npm run start:performanceand go to the grid with 1 million records. Sort thePositioncolumn. Click on the first cell of theRegisteredcolumn. An error is thrown with maximum callstack exceeded.
3. For 1 million records on the performance sample, it takes 4 to 5 seconds to merge the cells. Sometimes the browser crashes out of memory. This makes all other features that are pipe related to feel very slow.
|
|
||
| } | ||
| result.splice(index, res.length, ...res); | ||
| result = result.slice(0, index).concat(res, result.slice(index + res.length)); |
There was a problem hiding this comment.
Could we use a regular for-of loop here and instead of creating 3 new array coppies we can mutate the result (which is cloned beforehand) in place?
There was a problem hiding this comment.
Do you mean going through all items in res and replacing each one in the original array with the unmerged version?
There was a problem hiding this comment.
I mean, we could. But it would look a bit ugly and also not sure how much more performant it would be considering that in the particular scenario:
- Go to the performance project and enable the merging on all columns. Run the performance samples using
npm run start:performanceand go to the grid with 1 million records. Sort thePositioncolumn. Click on the first cell of theRegisteredcolumn. An error is thrown with maximum callstack exceeded.
It would need to loop through 300k+ records and replace each one.
There was a problem hiding this comment.
I thought that
for(let i =0;i<res.length;++i) {
result[index+i] = res[i];
}
would be faster because there is no array copying but changes the result in place. However, seems like the slice and concat version is perhaps more optimized by the js engine. I have tested this and, indeed, your current version is faster.
tishko0
added
✅ status: verified
|
mouse wheel when there is cell merging is becoming choppy, it was faster before the changes and there is a difference when compared to a grid without cell merging |
@dkamburov I can confirm that none of the merge related pipes are actually triggered on mouse wheel, so it doesn't make sense for the change to effect it. Are you sure it's not simply because of the large amount of data in the grid from the sample? It's choppy for me even without merging. |
|
Seems like that's due to the huge amount of data in that sample(same behavior without merging) |
Closes #16146
Split pipes in two, where one does the merging and the other tracks active rows and unmerges within related merge sequences.
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)